@prantogg Does this work with the --mb-per-file 256 config?

Thank you!

I don't think it is necessary for this PR, but the ObjectStore may be a good future choice (here it would let you unify the local and s3 code path and make it easier to add future gcs or azure support).

It may be worth setting up a test for this in CI using MinIO or another S3 alternative as otherwise I don't think there are any tests here to validate the behaviour.

Thanks @paleolimbot! Agreed on ObjectStore for local path. The current implementation for local path is an exact copy of tpchgen-rs project. Happy to tackle that as a follow-up!

On the MinIO CI test - my thinking was that since S3Writer implements std::io::Write, the datagen pipeline is agnostic to the destination. The unit tests validate the writer logic (buffering, multipart, finalization) against an InMemory object store, which covers our code. Testing against an actual S3 endpoint would mostly be validating object_store::aws::AmazonS3Builder's backend rather than ours?

Adding a small fix for S3 error propagation for things like token expiry.

@prantogg Does this work with the --mb-per-file 256 config?

@jiayuasu Yes, it works with all CLI options including --mb-per-file. Data is just streamed to S3 via multipart upload instead of written to disk.

Testing against an actual S3 endpoint would mostly be validating not our code

There's nothing in this PR to indicate that spatialbench-cli -s 1 --format=parquet --output-dir s3://my-bucket/sf1-parquet actually runs (and nothing to ensure that future LLMs or contributors won't break it). I call this an "is it plugged in check" and I while I think future you will be glad you added it now, the consequences of this not working are very low (and not my problem 🙂 ) so feel free to punt!

There's nothing in this PR to indicate that spatialbench-cli -s 1 --format=parquet --output-dir s3://my-bucket/sf1-parquet actually runs (and nothing to ensure that future LLMs or contributors won't break it). I call this an "is it plugged in check" and I while I think future you will be glad you added it now, the consequences of this not working are very low (and not my problem 🙂 ) so feel free to punt!

I see your point @paleolimbot! Thanks for that. Just added MinIO tests